Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BUG: Don't import helpers in namespaces #302

Merged
merged 1 commit into from
Apr 8, 2025

Conversation

crusaderky
Copy link
Contributor

@crusaderky crusaderky commented Apr 3, 2025

Remove helper functions from wrapped array namespaces.
This was introduced when the numpy namespace was split from the helpers in 2022 (b3a12d9).
This PR prompted by a collision between array_api_compat.device and torch.device.

Re. deprecation layer: unsure if we need one? This was never advertised in the documentation.

@@ -174,3 +174,5 @@ def trace(
'svd', 'cholesky', 'matrix_rank', 'pinv', 'matrix_norm',
'matrix_transpose', 'svdvals', 'vecdot', 'vector_norm', 'diagonal',
'trace']

_all_ignore = ['math', 'normalize_axis_tuple', 'get_xp', 'np', 'isdtype']
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No idea why test_all wasn't failing before.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, test_all is a bit brittle. At a guess, its iterating over sys.modules is meant to be robust against renames/new private submodules in array_api_compat/*. But then it's not easy to know if something was or was not imported.

@crusaderky crusaderky changed the title [WIP] BUG: Don't import helpers in namespaces BUG: Don't import helpers in namespaces Apr 3, 2025
@crusaderky crusaderky marked this pull request as ready for review April 3, 2025 14:04
@crusaderky
Copy link
Contributor Author

This is ready for review.
I'm personally against building a cumbersome deprecation layer.
Nominally, because I expect that anybody who tests vs. a battery of backends will include array-api-strict in it, which does not contain any of the helper functions in its namespace.
So this would exclusively cause a regression for people who very specifically support only the four backends wrapped by array-api-compat and nothing else.

@@ -276,7 +276,7 @@ def test_asarray_copy(library):
is_lib_func = globals()[is_array_functions[library]]
all = xp.all if library != 'dask.array' else lambda x: xp.all(x).compute()

if library == 'numpy' and xp.__version__[0] < '2' and not hasattr(xp, '_CopyMode') :
if library == 'numpy' and xp.__version__[0] < '2' and not hasattr(np, "_CopyMode"):
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI this is for numpy 1.21

Copy link
Member

@ev-br ev-br left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. +1 for not needing a deprecation layer. Thanks @crusaderky

@@ -174,3 +174,5 @@ def trace(
'svd', 'cholesky', 'matrix_rank', 'pinv', 'matrix_norm',
'matrix_transpose', 'svdvals', 'vecdot', 'vector_norm', 'diagonal',
'trace']

_all_ignore = ['math', 'normalize_axis_tuple', 'get_xp', 'np', 'isdtype']
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, test_all is a bit brittle. At a guess, its iterating over sys.modules is meant to be robust against renames/new private submodules in array_api_compat/*. But then it's not easy to know if something was or was not imported.

@ev-br ev-br merged commit 3e5fdc0 into data-apis:main Apr 8, 2025
40 checks passed
@ev-br ev-br added this to the 1.12 milestone Apr 8, 2025
@crusaderky crusaderky deleted the import_helpers branch April 8, 2025 17:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants